Skip to content

refactor: extract shared TROP bootstrap resampling loop#554

Merged
igerber merged 1 commit into
mainfrom
refactor/trop-bootstrap-loop-dedup
Jun 26, 2026
Merged

refactor: extract shared TROP bootstrap resampling loop#554
igerber merged 1 commit into
mainfrom
refactor/trop-bootstrap-loop-dedup

Conversation

@igerber

@igerber igerber commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • Deduplicate the byte-identical per-draw "pairs bootstrap" resampling loop shared by TROP._bootstrap_variance (trop_local.py) and TROP._bootstrap_variance_global (trop_global.py) into a new module-level helper _run_trop_bootstrap_loop in trop_local.py (imported by trop_global.py, mirroring the existing _setup_trop_data pattern). The two loops differed only in the per-draw refit call, now passed as a fit_callable closure.
  • Remove the resolved TROP bootstrap-dedup row from TODO.md.

This is a pure, behavior-preserving refactor: RNG setup and the Rust-backend dispatch stay inline (the helper is RNG-free; the stratified bootstrap indices are pre-generated before dispatch, so the Rust/Python cross-backend parity is preserved); the post-loop warnings — whose wording legitimately differs between local and global — and the np.std(ddof=1) SE computation stay inline in each method; and the closures resolve self._fit_*_with_fixed_lambda at call time, so the existing monkeypatching tests still intercept.

Methodology references

  • Method name(s): N/A — no methodology change. TROP variance remains bootstrap-based exactly as before (unit-level pairs resampling, fixed-lambda refits, failure filtering, proportional-failure warnings, NaN SE on zero successful draws). The REGISTRY TROP section is unchanged.
  • Paper / source link(s): N/A (no algorithm change; the TROP / Algorithm 3 bootstrap description is unchanged).
  • Intentional deviations from the source: None. One documented implementation micro-detail: in the degenerate empty-pool branch, the empty-array dtype unifies to control_units.dtype. This branch is unreachable in a real fit (_setup_trop_data rejects an empty treated/control pool before the bootstrap runs), and the resulting boot_data is byte-identical regardless since f"{u}_{idx}" coerces every unit id to str.

Validation

  • Bit-identical verified: local + global bootstrap SE and the full bootstrap distributions match exactly pre/post on a fixed seed (Python-fallback path), confirmed by a before/after capture.
  • Tests: full tests/test_trop.py + tests/test_rust_backend.py::TestTROPRustEdgeCaseParity = 119 passed (including the atol=1e-14 global / 1e-5 local bootstrap seed-reproducibility parity tests and the NaN-SE / proportional-failure-rate guards). Added 6 direct unit tests for the helper (tests/test_trop.py::TestRunTropBootstrapLoop): resampling + renamed ids, finite filter, exception skip, non-convergence tracker passthrough, and the empty-pool branches.
  • black / ruff / mypy clean on the changed lines (no new errors).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

@igerber @claude
refactor: extract shared TROP bootstrap resampling loop
7d51a02 
Deduplicate the byte-identical per-draw pairs-bootstrap loop in
TROP._bootstrap_variance (trop_local.py) and
TROP._bootstrap_variance_global (trop_global.py) into a shared module-level
helper _run_trop_bootstrap_loop in trop_local.py (imported by trop_global.py,
mirroring the existing _setup_trop_data pattern). The two loops differed only
in the per-draw refit call, now passed as a fit_callable closure.

Pure bit-identical refactor - no estimate / SE / RNG / warning change:
- RNG setup and the Rust-backend dispatch stay inline (the helper is RNG-free;
  indices are pre-generated before dispatch, so cross-backend parity holds).
- the post-loop warnings (whose wording legitimately differs between local and
  global) and np.std(ddof=1) stay inline in each method.
- the closures resolve self._fit_*_with_fixed_lambda at call time, so the tests
  that monkeypatch them still intercept.

Verified bit-identical: local + global bootstrap SE and the full bootstrap
distributions match exactly pre/post on a fixed seed (Python fallback). Full
TROP suite + Rust parity gate (119 tests, incl. atol=1e-14/1e-5 bootstrap
seed-reproducibility) pass; 6 new direct unit tests cover the helper.

Closes the TROP bootstrap-dedup TODO row. Behavior-preserving, so no
REGISTRY / CHANGELOG change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected methods: TROP local _bootstrap_variance and global _bootstrap_variance_global.
  • Methodology check passes: the refactor preserves Algorithm 3-style treatment-stratified unit/block bootstrap, fixed-lambda refits, finite-draw filtering, failure warnings, zero-success NaN SE behavior, and np.std(..., ddof=1).
  • No new inline inference anti-patterns or partial NaN guards found in the changed TROP paths.
  • Added helper tests cover resampling/renaming, finite filtering, exception skipping, non-convergence tracker passthrough, and empty-pool branches.
  • git diff --check passed. I could not run pytest because this environment lacks pytest and pandas.

Methodology

Finding: P3 informational — behavior-preserving TROP bootstrap extraction

  • Impact: No methodology defect found. The registry requires TROP standard errors to use block bootstrap preserving panel structure and notes zero-success bootstrap SE should be NaN with downstream safe_inference() propagation. The changed code still pre-generates treatment-stratified bootstrap indices in each caller, resamples whole unit time series with replacement, refits at fixed lambda through the closure, filters non-finite estimates, warns on high failure rates, returns NaN for zero successful draws, and computes SE with ddof=1. See docs/methodology/REGISTRY.md:L2582-L2611, docs/methodology/REGISTRY.md:L2740-L2741, diff_diff/trop_local.py:L1114-L1225, diff_diff/trop_global.py:L914-L1028, and diff_diff/trop_local.py:L197-L254.
  • Concrete fix: None required.

Code Quality

Finding: None

  • Severity: N/A
  • Impact: The extraction is scoped and reuses the existing trop_global.py importing-from-trop_local.py pattern. The helper’s call contract is explicit and keeps method-specific warnings/SE handling at the caller.
  • Concrete fix: None required.

Performance

Finding: None

  • Severity: N/A
  • Impact: The helper does not add RNG work or change Rust dispatch. Python fallback still performs the same per-draw pd.concat loop as before.
  • Concrete fix: None required.

Maintainability

Finding: P3 informational — TODO cleanup matches implemented work

  • Impact: Removing the TROP bootstrap-dedup TODO is appropriate because the shared loop now exists at diff_diff/trop_local.py:L197-L254 and is used by both local and global fallback paths.
  • Concrete fix: None required.

Tech Debt

Finding: None

  • Severity: N/A
  • Impact: No new deferred correctness issue was introduced. Remaining TROP TODO rows are separate Rust solver/RNG parity follow-ups, not caused by this PR.
  • Concrete fix: None required.

Security

Finding: None

  • Severity: N/A
  • Impact: No secrets, unsafe deserialization, shell execution, or external I/O changes found in the diff.
  • Concrete fix: None required.

Documentation/Tests

Finding: P3 informational — targeted tests added; local execution unavailable here

  • Impact: The new TestRunTropBootstrapLoop coverage exercises the helper’s key behavior directly at tests/test_trop.py:L2868-L3027. I could not independently run it because /usr/bin/python has no pytest or pandas installed in this review environment.
  • Concrete fix: None required for the PR; run the targeted TROP tests in CI or a dev environment with project dependencies installed.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jun 26, 2026
@igerber igerber merged commit 2720d98 into main Jun 26, 2026
33 of 34 checks passed
@igerber igerber deleted the refactor/trop-bootstrap-loop-dedup branch June 26, 2026 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igerber